Can you explain why this works? Say you have 5 splits:

• 1 is contained in the request
• 4 are only overlapping the request

In that case min_required_splits would be 1 here, but there might be a biggest_end_timestamp or smallest_start_timestamp in any of the other splits.

My thoughts were that there's a

.take_while(|partial_sum| *partial_sum < num_requested_docs)

so min_required_splits would not get to be 1, but now I see that I should validate that we actually reached this condition, and only if we reached it to set min_required_splits and optimize, otherwise return early.

I pushed a quick fix, I need to test it, but I think it answers the problem.

I didn't know of std::ops::ControlFlow 😄. Nevertheless, this reaches the limit when iterators stop being practical. A for loop is much more readable here.

nit, this is more readable

min_required_splits += 1;
if partial_sum >= num_requested_docs {
    return Some(min_required_splits);
}

nice, this is now really straightforward to understand and verify 😄

It is really hard to know here if contained splits come first or second (the ordering of boolean is not obvious, the ! negation doesn't help 😄 ). Could we maybe explicitly map true/false to something that is more clearly ordered:

let contained_first = match Self::is_split_contained_in_search_time_range(split, &request) {
  true => 0u8,
  false => 1u8,
};
(contained_first, std::cmp::Reverse(split.timestamp_end()))

MORE IMPORTANT: now that I think about it, I thing it's not good idea to have contained splits processed first in the general case (!is_simple_all_query or get_min_required_splits().is_none()`). You miss an optimization opportunity, because once you'll have processed them, you will still need to check the splits overlapping the higher end of the range for better matches.

hmm, we can maybe instead of sorting by contained, keep the same sort mechanism as it was.

but still keep the new implementation of get_min_required_splits, meaning we won't optimize the first few splits which might not be contained in the search query's range.

made the change to not sort, tell me what you think.

keep the comment:

// TODO: we maybe want here some deduplication + Cow logic

we could actually call to_splits_with_request before (inside optimize) and simplify this signature to something like:

This would prevent quite a lot of repetition and is also more explicit because it shows that we don't drop splits (note that &self is also dropped because not used)
(applies to all optimzie_split_xxx)

I modified to remove &self, tried what you suggested, but then simple stuff like calling is_simple_all_query that accepts a &SearchRequest became ugly, WDYT?